Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for webContents option in BrowserView #26802

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

sentialx
Copy link
Contributor

@sentialx sentialx commented Dec 2, 2020

Description of Change

This PR adds support for webContents option in BrowserView, which works similarly as in BrowserWindow. Since the new webContents.setWindowOpenHandler doesn't support BrowserViews, we can at least listen to the internal -add-new-contents event and create a BrowserView with provided webContents. This PR will also help in adding support for BrowserView in setWindowOpenHandler in the future.

Closes #24833

Could this be backported to 12-x-y?

Checklist

Release Notes

Since it's a hidden API, I think it's not worth mentioning this change in the release notes.

Notes: no-notes

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Dec 2, 2020
@codebytere codebytere requested a review from a team December 2, 2020 17:58
@nornagon nornagon added the semver/minor backwards-compatible functionality label Dec 4, 2020
@nornagon
Copy link
Member

nornagon commented Dec 8, 2020

I'm not sure how I understand the way this would work—would you pass show: false in the BrowserWindowConstructor options in setWindowOpenOverrideHandler, so the BrowserWindow would still be created behind the scenes but hidden, and then yank the WebContents out of it and put it in a BrowserView? That seems like it might cause other problems 🤔

@sentialx
Copy link
Contributor Author

sentialx commented Dec 8, 2020

It allows us to override the internal -add-new-contents event and implement our own way to create a BrowserView

  tab.webContents.removeAllListeners('-add-new-contents' as any);
  tab.webContents.on(
    '-add-new-contents' as any,
    (
      event: any,
      webContents: Electron.WebContents,
      disposition: string,
      _userGesture: boolean,
      _left: number,
      _top: number,
      _width: number,
      _height: number,
      url: string,
      frameName: string,
      referrer: Electron.Referrer,
      rawFeatures: string,
      postData: PostData,
    ) => {
      if (
        disposition !== 'foreground-tab' &&
        disposition !== 'new-window' &&
        disposition !== 'background-tab'
      ) {
        event.preventDefault();
        return;
      }
      new BrowserView({ webContents });
    },
  );

@nornagon nornagon added semver/patch backwards-compatible bug fixes and removed api-review/requested 🗳 semver/minor backwards-compatible functionality labels Dec 9, 2020
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours and removed new-pr 🌱 PR opened in the last 24 hours labels Dec 9, 2020
@nornagon
Copy link
Member

nornagon commented Dec 9, 2020

Given this API is intended to be undocumented, I'm downgrading from semver/minor to semver/patch and removing the API review request.

JFYI, we don't support overriding -add-new-contents and it's entirely possible that this workaround will break in future. I'd encourage you to define and document a public API that accomplishes this if you want this functionality to be stable.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests, thanks!

@sentialx
Copy link
Contributor Author

@nornagon done.

@sentialx
Copy link
Contributor Author

Sorry for changing my mind, but would it be possible to also backport to 10 and 11?

@zcbenz
Copy link
Member

zcbenz commented Dec 15, 2020

Sorry for changing my mind, but would it be possible to also backport to 10 and 11?

This is for @electron/wg-releases to decide, I'll add related tags.

@zcbenz zcbenz merged commit 1e2a200 into electron:master Dec 15, 2020
@release-clerk
Copy link

release-clerk bot commented Dec 15, 2020

No Release Notes

@trop
Copy link
Contributor

trop bot commented Dec 15, 2020

I was unable to backport this PR to "10-x-y" cleanly;
you will need to perform this backport manually.

sentialx added a commit to sentialx/electron that referenced this pull request Dec 21, 2020
* feat: add support for webContents option in BrowserView

* tests: add tests
@trop
Copy link
Contributor

trop bot commented Dec 21, 2020

@sentialx has manually backported this PR to "11-x-y", please check out #27095

@trop trop bot removed the in-flight/10-x-y label Jan 5, 2021
zcbenz pushed a commit that referenced this pull request Jan 5, 2021
* feat: add support for webContents option in BrowserView (#26802)

* feat: add support for webContents option in BrowserView

* tests: add tests

* fix: missing webContents import

* fix: WebContents::New -> Create
@PalmerAL
Copy link
Contributor

PalmerAL commented Jan 5, 2021

Is there any plan to turn this into a public API? This would be really useful for me, but I'm concerned about depending on it as long as it remains undocumented.

zcbenz pushed a commit that referenced this pull request Jan 5, 2021
)

* feat: add support for webContents option in BrowserView

* tests: add tests
@pOoOf
Copy link

pOoOf commented May 8, 2021

Hey guys, i am trying to use this API to open a browserview as a guest window upon window.open and this is what i am doing to make it work

rightPaneWindowRef.webContents.removeAllListeners('-add-new-contents');
rightPaneWindowRef.webContents.on(
  '-add-new-contents',
  (
    newViewevent,
    webContents,
    disposition,
    _userGesture,
    _left,
    _top,
    _width,
    _height,
    newViewurl
  ) => {
    console.log({ disposition, newViewurl });
    const newView = new BrowserView({
      ...browserViewConfig,
      webContents,
    });
    mainWindow.addBrowserView(newView);
    newView.setBounds({
      x: leftViewWidth + 100,
      y: HEADER_HEIGHT + 100,
      width: 600,
      height: 600,
    });
  }
);

Now this works fine but when i try to close the BrowserView by clicking on any option on the UI(it may be calling window.opener), the application crashes.

Can anyone tell me what i could be doing wrong here?

Also what @sentialx suggested doesn't work, this code below doesn't open any BrowserView and nothing is visible on the UI utill i add this BrowserView into the BrowserWindow using the addBrowserView method as shown above

tab.webContents.removeAllListeners('-add-new-contents' as any);
  tab.webContents.on(
    '-add-new-contents' as any,
    (
      event: any,
      webContents: Electron.WebContents,
      disposition: string,
      _userGesture: boolean,
      _left: number,
      _top: number,
      _width: number,
      _height: number,
      url: string,
      frameName: string,
      referrer: Electron.Referrer,
      rawFeatures: string,
      postData: PostData,
    ) => {
      if (
        disposition !== 'foreground-tab' &&
        disposition !== 'new-window' &&
        disposition !== 'background-tab'
      ) {
        event.preventDefault();
        return;
      }
      new BrowserView({ webContents });
    },
  );

@PalmerAL
Copy link
Contributor

I also tried this out (code), and am running into similar crashes when closing the view. I suspect this just doesn't work 🤷

However, @sentialx I'm assuming you have this working in your new browser, so if there's a secret to getting this to work, I'd be very interested to know what it is.

@sentialx
Copy link
Contributor Author

By closing do you mean calling window.close or just removing the view from window?

@PalmerAL
Copy link
Contributor

In my case, removing the view from the window and calling webContents.destroy() (code). It doesn’t crash every time though, so there may be something else going on. I’ll put together a testcase sometime in the next couple days and see what I can figure out.

@pOoOf
Copy link

pOoOf commented May 12, 2021

Now this works fine but when i try to close the BrowserView by clicking on any option on the UI(it may be calling window.opener), the application crashes.

When i said the application crashes, the application wasn't actually crashing which i later realized, what actually was happening was that, the new BrowserView(new Guest window) was closing the mainWindow upon calling window.close, i guess this is also an existing issue when we use BrowserView as a guest window

I solved this issue by adding a preload script to the BrowserView, which overrides window.open and rather sends a sync event to the main process which then removes the BrowserView using removeBrowserView method on BrowserWIndow instance.
Also to solve this issue, i also have to enable contextIsolation in the BrowserView, which i don't think is recommended.

When this issue will be resolved, this hack won't be needed anymore.

@PalmerAL
Copy link
Contributor

In my case, it is actually crashing, but after testing it again, I'm only seeing the crash infrequently (maybe 1/20 view closes?). I may still try to make a testcase at some point, but I don't think I can until I find a way to reproduce it consistently.

I don't have an actual crash report; this is what I get as console output:

[0514/162332.291416:WARNING:process_memory_mac.cc(93)] mach_vm_read(0x7ffee6d15000, 0x2000): (os/kern) invalid address (1)
[1] [0514/162332.520512:WARNING:crash_report_exception_handler.cc(240)] UniversalExceptionRaise: (os/kern) failure (5)

But that's probably not very helpful for diagnosing.

@zerooverture
Copy link

zerooverture commented Oct 28, 2021

-add-new-contents generated webContents add -add-new-contents event listener invalid
example @sentialx

I found out why, BrowserView passed webContents must pass webPreferences.nativeWindowOpen = true

tab.webContents.removeAllListeners('-add-new-contents');
  tab.webContents.on( '-add-new-contents', (event, webContents, disposition) => {
      if (
        disposition !== 'foreground-tab' &&
        disposition !== 'new-window' &&
        disposition !== 'background-tab'
      ) {
        event.preventDefault();
        return;
      }
     // const newView = new BrowserView({ webContents });
     // newView.webContents.on('-add-new-contents',()=>{
     //    console.log("testing"); // No response to this listener
     // })
       new BrowserView({ webContents,webPreferences: {nativeWindowOpen: true} });
    },
  );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/approved ✅ semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Allow BrowserView to be a newGuest in new-window
9 participants